-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: make SchemaRegistry permission validations on KSQL requests #7773
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just one question
@@ -196,8 +197,7 @@ private static SchemaResult notFound(final String topicName, final boolean isKey | |||
+ "\t-> Use the REST API to list available subjects" | |||
+ "\t" + DocumentationLinks.SR_REST_GETSUBJECTS_DOC_URL | |||
+ System.lineSeparator() | |||
+ "- You do not have permissions to access the Schema Registry.Subject: " + subject | |||
+ System.lineSeparator() | |||
+ "- You do not have permissions to access the Schema Registry." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to tell if the exception was due to an auth error? If so, would it be possible to return a separate exception for auth errors so that the error messages can have more specific information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this was added this way. I will look at it in a follow-up PR.
Will re-review after more permissions checks are added
8f0d6de
to
aac0500
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find the class KsqlSchemaAuthorizationException
, is it missing in this PR?
* An implementation of {@link KsqlAccessValidator} that provides authorization checks | ||
* from a external authorization provider. | ||
*/ | ||
public class KsqlProvidedAccessValidator implements KsqlAccessValidator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not just name it KsqlAccessValidatorImpl
? KsqlProvidedAccessValidator
sounds a bit weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 3 classes that extend from this:
KsqlProvidedAccessValidator
KsqlCacheAccessValidator
KsqlBackendAccessValidator
I used Provided...
because it is a validator provided by the user on the ksql-server.properties
. I wasn't sure if use this os KsqlExternalAccessValidator
. Any ideas?
extractor.process(query, null); | ||
for (String kafkaTopic : extractor.getSourceTopics()) { | ||
accessValidator.checkAccess(securityContext, kafkaTopic, AclOperation.READ); | ||
for (KsqlTopic ksqlTopic : extractQueryTopics(query, metaStore)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a comment but a question for my own understanding: it seems KsqlAuthorizationValidator
is wrapping a KsqlAccessValidator
here, can we just merge these two interfaces? I guess that's boiled down to:
- What are the conceptual differences of these two?
- Would
KsqlAccessValidator
ever need to be leveraged directly without theKsqlAuthorizationValidator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KsqlAuthorizationValidator
validates the user can execute a specified statement. This class gets information about that, such as topics and schemas; and then validates each resource based on the type of statement (query, create, insert, ...).
Initially, the validation was part of this KsqlAuthorizationValidator
, but I split it when I introduced an authorization cache, which lives in KsqlCacheAccessValidator
. The cache wraps another validation if the cache is enabled. The current validation, besides caching, is the KsqlBackendAccessValidator
, which just checks with the backend service (i.e. Kafka) to check for ACLs. The new one is KsqlProvidedAccessValidator
, which checks with an external service, like RBAC. SR permissions will be checked by the KSQL confluent security plugins which can either use RBAC or call the SR /permissions
endpoint to check for ACLs (both part of the confluent security plugins).
@guozhangwang The |
) { | ||
if (externalAuthorizationProvider.isPresent()) { | ||
return Optional.of(new KsqlProvidedAccessValidator(externalAuthorizationProvider.get())); | ||
} else if (isTopicAccessValidatorEnabled(ksqlConfig, serviceContext)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have a config for enabling SR auth?
…uthorizationException (confluentinc#7783)
Description
KSQL has already validations for Kafka permissions when a new query, DDL or DML statement is executed by a user. This PR adds another validation for Schema Registry permissions.
The idea behind this PR is to let users know exactly what's causing a query or DDL operation to fail and also produce a more generic authorization error message similar to the one we have with Kafka. Currently, some statements show the SR authorization error returned by SR, but queries, for instance, never shown anything. Queries run and do not show any error and users always wonder why there's no data streamed (reason was a SR denied operation).
NOTE
This PR is based on an very old PR (#3323). It was easier to push a new PR with the updates. Also, the comments in that PR suggested we change the SR API to produce the authorization error we want, but that requires more work in SR and KSQL. Also, SR may change error messages in the future. The idea of this PR is to have our own authorization message error, for both SR and Kafka, and no depend from other services when we implement better authorization alternatives (i.e. check against RBAC instead of SR and Kafka).
To help the reviewer understand the workflow of validations.
Start on the
KsqlAuthorizationValidatorImpl
. This validator is called on every KSQL request to check a user has permissions to access a topic (and now a SR subject). It is also called by the DistributedExecutor before writing a command to the command topic to check if the KSQL server will have permissions to access the topic (and now a SR subject) later when the command is processed. If the user or the KSQL server do not have permissions, then it throws an authorization exception. No all perms checks are done in this class. Other perms are checked when calling the SR client directly, which should return the right authorization exception.The new authorization exception for SR is called
KsqlSchemaAuthorizationException
that produces an error message with the denied operation. This is the message the users will see in the CLI. The message is similar to theKsqlTopicAuthorizationException
.The
KsqlAuthorizationValidatorImpl
calls the internal backed validationKsqlBackendAccessValidator
, which now has a SR validation. The SR validation only supports READ operations. WRITE operations are not possible yet (need to figure out how). We might need to make changes in SR to have a new API to check permissions, but this may require a design doc for SR to add new REST API (will follow-up later). Other option is to check the permission directly in RBAC, but requires changes in the security plugins (will follow-up in another PR).The
KsqlCacheAccessValidator
is also updated to cache SR permissions checks. If you ask why a cache?, this cache was implemented in the past to make pull queries perform better.The rest of the changes are places where the SR client is called to fetch or register a schema. Those places just make sure to return the
KsqlSchemaAuthorizationException
in case the returned SR error is 403 or 404.Testing done
Added some unit tests and verified in an environment with RBAC + SR.
Reviewer checklist